Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[5.3] SEF: Fix URLs when preprocessing #43992

Merged
merged 12 commits into from
Oct 29, 2024
Merged

[5.3] SEF: Fix URLs when preprocessing #43992

merged 12 commits into from
Oct 29, 2024

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented Aug 28, 2024

Summary of Changes

The routing in Joomla requires some information to be in the query to build proper URLs. One is the slug, the combination of the ID and alias of a content item (for example &id=42:the-answer-to-everything) and the other is the parent key, if the extension requires this. (for example &catid=21) When this information is missing, the router can't build the right URL.

This PR adds a new component router rule, which should help fixing such URLs. The rule is supposed to be flexible enough to be used by most third party components. The parameters for the rule are the view configuration to act upon, the table to read the info from, the tables key and the tables parent key. The parent key is optional and the rule can be added more than once for different views.

Testing Instructions

  1. You need at least one article which is not directly linked to by a menu item.
  2. Edit an article and insert a link like index.php?option=com_content&view=article&id=<id> where id is the ID of the article which is NOT directly linked to by a menu item.
  3. Check out the link in the frontend by visiting the article.

Actual result BEFORE applying this Pull Request

You get a strange link, for example something like /menuitem?view=article&id=42

Expected result AFTER applying this Pull Request

You get the correct link, for example /menuitem/this-test-is-successfull

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@dautrich
Copy link

I have tested this item 🔴 unsuccessfully on da8002a

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43992.

@dautrich
Copy link

My test wasn't successful. Here the screenshots:

Before applying the patch
Before_linking_article
Before_linked_article

After applying the patch
After_linking_article
After_linked_article

@Hackwar
Copy link
Member Author

Hackwar commented Aug 28, 2024

As this PR is against 5.2, you have to also test this on 5.2-dev and not 5.1.

@Hackwar
Copy link
Member Author

Hackwar commented Aug 28, 2024

When you link the category of the article in a menu item, you should have successfull tests.

@Hackwar
Copy link
Member Author

Hackwar commented Aug 28, 2024

Since we are in feature freeze, I'm going to change the target of this PR to 5.3-dev.

@Hackwar Hackwar changed the base branch from 5.2-dev to 5.3-dev August 28, 2024 12:49
@Hackwar Hackwar changed the title [5.2] SEF: Fix URLs when preprocessing [5.3] SEF: Fix URLs when preprocessing Aug 28, 2024
@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-5.3-dev labels Aug 28, 2024
@dautrich
Copy link

As this PR is against 5.2, you have to also test this on 5.2-dev and not 5.1.

@Hackwar Can I use 5.2beta1 or do I need a nightly build?

@dautrich
Copy link

When you link the category of the article in a menu item, you should have successfull tests.

@Hackwar ???

@richard67
Copy link
Member

@Hackwar System tests are failing:

Running:  site/components/com_contact/Category.cy.js                                   (48 of 124)

  Test in frontend that the contact category view
    ✓ can display a list of contacts in a menu item (1282ms)
    ✓ can display a list of contacts without a menu item (663ms)
    1) "after each" hook for "can open the contact form in the default layout"

  2 passing (5s)
  1 failing

  1) Test in frontend that the contact category view
       "after each" hook for "can open the contact form in the default layout":
     Error: Unwanted PHP Warning: "  Undefined array key 1 in <b>/tests/www/cmysql/components/com_contact/src/Service/Router.php</b> on line <b>163</b>"

Because this error occurred during a `after each` hook we are skipping all of the remaining tests.

@Hackwar
Copy link
Member Author

Hackwar commented Aug 28, 2024

@dautrich beta1 should be enough.

@richard67 I'm aware. The tests are broken, since they are saving the contact with a broken catid.

@joomla-cms-bot joomla-cms-bot removed the Language Change This is for Translators label Aug 28, 2024
@Hackwar Hackwar mentioned this pull request Aug 29, 2024
Merged
4 tasks
@dautrich
Copy link

dautrich commented Aug 31, 2024 •
edited
Loading

I have tested this item ✅ successfully on c57d285

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43992.

Tested on naked 5.2.0-beta1, local with Laragon.

@Elfangor93
Copy link
Contributor

Elfangor93 commented Sep 30, 2024 •
edited
Loading

When applying this PR using the patch tester component, the Link to the article as well as its URL changes from

SEF Plugin, Strict Routing: No
index.php/menuitem?view=article&id=2 (before this PR)
to
index.php/menuitem?view=article&id=2:autos&catid=2 (after this PR)

SEF Plugin, Strict Routing: Yes
index.php/component/content/article/autos (before this PR)
to
index.php/component/content/article/autos?catid=2 (after this PR)

Joomla v5.2.0-beta3
PHP v8.1.2

@Elfangor93
Copy link
Contributor

I have tested this item ✅ successfully on 894383e

See results of my test in the above comment. In consultation with the author of the PR the resulting behavior is as expected. Even if the case with activated strict routing in the SEF Plugin produces a bad result.

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43992.

@SniperSister
Copy link
Contributor

I have tested this item ✅ successfully on 894383e

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43992.

@alikon
Copy link
Contributor

alikon commented Oct 4, 2024

RTC

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43992.

@joomla-cms-bot joomla-cms-bot added RTC This Pull Request is Ready To Commit and removed Unit/System Tests labels Oct 4, 2024
@rdeutz rdeutz merged commit 188f40c into joomla:5.3-dev Oct 29, 2024
3 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 29, 2024
@rdeutz
Copy link
Contributor

rdeutz commented Oct 29, 2024

Thanks to all who have worked on this PR.

@rdeutz rdeutz added this to the Joomla! 5.3.0 milestone Oct 29, 2024
This was referenced Nov 14, 2024
Merged
Open
@Hackwar Hackwar mentioned this pull request Nov 18, 2024
Merged
4 tasks
@Hackwar Hackwar mentioned this pull request Nov 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brianteeman brianteeman brianteeman left review comments

Assignees
No one assigned
Labels
Feature PR-5.3-dev
Projects
None yet
Milestone
Joomla! 5.3.0
Development

Successfully merging this pull request may close these issues.
10 participants
@Hackwar @dautrich @richard67 @Elfangor93 @SniperSister @alikon @rdeutz @brianteeman @bembelimen @joomla-cms-bot